Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replaced TinyMCE7 with TinyMCE6 #2

Merged
merged 5 commits into from
Oct 28, 2024

Conversation

fballiano
Copy link

I couldn't test an installation with this but I know for sure tiny6 is compatible with tiny7 (did the same tests for my own projects)

@rhoerr
Copy link
Owner

rhoerr commented Oct 24, 2024

Awesome, thank you.

I would like to also incorporate these fixes to upstream bugs introduced by the tiny7 upgrade:
magento/magento2#39253
magento/magento2#39258

And possibly incorporate changes from magefan/module-wysiwyg-advanced@master...ivanhrytsaim:module-wysiwyg-advanced:12062-TinyMCE-support-upgrade to enable the font size option (also an upstream regression with tiny7).

@fballiano
Copy link
Author

I would like to also incorporate these fixes to upstream bugs introduced by the tiny7 upgrade: magento/magento2#39253 magento/magento2#39258

done

And possibly incorporate changes from magefan/module-wysiwyg-advanced@master...ivanhrytsaim:module-wysiwyg-advanced:12062-TinyMCE-support-upgrade to enable the font size option (also an upstream regression with tiny7).

but where should I put that file? I can't find anything related to that code in our repo

@rhoerr
Copy link
Owner

rhoerr commented Oct 24, 2024

but where should I put that file? I can't find anything related to that code in our repo

I'll investigate that later -- either it's hiding somewhere in the core WYSIWYG code, or core doesn't initialize these settings at all currently.

@rhoerr
Copy link
Owner

rhoerr commented Oct 24, 2024

I found where core handles that config: \Magento\Cms\Model\Wysiwyg\DefaultConfigProvider::getConfig

Possibly related test config: https://github.com/mage-os/mageos-magento2/blob/2.4-develop/app/code/Magento/Cms/Test/Mftf/Section/TinyMCESection/TinyMCESection.xml (although we don't use the MFTF test suite ourselves)

@fballiano
Copy link
Author

@rhoerr
Copy link
Owner

rhoerr commented Oct 24, 2024

It should probably be a separate PR either way. We can process this as-is. Thanks Fabrizio!

@rhoerr
Copy link
Owner

rhoerr commented Oct 27, 2024

The Tiny 6 files look good to me. I fixed a typo in the config XML. Unfortunately it's not actually working for me as of yet. I get this in console on loading the editor for a CMS page:

editor_plugin.js:14 Uncaught TypeError: tinymce.create is not a function
    at editor_plugin.js:14:17
    at tinymceAdapter.js:92:29
    at Object.execCb (require.js:1696:33)
    at context.execCb (resolver.js:156:31)
    at Module.check (require.js:883:51)
    at Module.<anonymous> (require.js:1139:34)
    at require.js:134:23
    at require.js:1189:21
    at each (require.js:59:31)
    at Module.emit (require.js:1188:17)

So something isn't wired up. I haven't investigated far enough yet to determine what.

This might be specific to Page Builder. It seems to load on product edit.

@rhoerr
Copy link
Owner

rhoerr commented Oct 28, 2024

Disregard that last! My mistake, had out of date requirejs-config without the updated tinymce reference. Found that on code review, retested, works on Page Builder too now. Going to approve and merge.

Copy link
Owner

@rhoerr rhoerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, looks good and checks out on QA.

@rhoerr rhoerr merged commit eb06ce0 into rhoerr:feat/add-2.4.7-p3 Oct 28, 2024
6 of 7 checks passed
@fballiano fballiano deleted the feat/add-2.4.7-p3 branch October 28, 2024 08:09
@fballiano
Copy link
Author

@rhoerr so sorry about the tag mismatch!! weird! phpstorm changes the closing tags automatically 🧐

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants